fix(auth): offer the API-key login on server-mode admin 403s - #1569
Open
paoloantinori wants to merge 3 commits into
Open
fix(auth): offer the API-key login on server-mode admin 403s#1569paoloantinori wants to merge 3 commits into
paoloantinori wants to merge 3 commits into
Conversation
…sh#1568) Since debpalash#1525 the /system router sits behind require_admin, so an unauthenticated non-loopback browser gets 403 where it used to get 401. The SPA routed only 401s into the ov:auth-required flow, so a remote browser spammed 403s on /sysinfo, /model/status, /system/notifications forever (polling hooks retry infinitely) and was never offered the master-for-session exchange. Frontend: route a 403 whose detail names the admin API key into the existing apikey gate; other 403s (CSRF, desktop-only) stay plain errors because no key can satisfy them. Backend: _admin_gate_403() emits a mode-distinct detail — server mode keeps "loopback origin or admin API key required" (a key genuinely satisfies it), desktop mode answers "loopback origin required" (truthful: the credential checks only run under server mode, so routing desktop guests to a key form would loop forever). tests/test_auth_gate_detail_lockstep.py locks the literal contract across layers so a backend reword fails CI unless client.ts moves with it. Verified end-to-end on a live LAN deployment: gate renders after 3 initial 403s, key exchange produces a session, all polls 200.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthroughChangesThe backend now returns mode-specific admin-gate 403 details. Server-mode API-key errors enter the frontend authentication flow. Desktop, CSRF, and unrelated 403 responses remain ordinary errors. Tests and documentation cover the behavior. Admin 403 authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change routes only fixable server-mode admin 403 responses into API-key authentication while preserving plain errors for cases a key cannot resolve; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
| Filename | Overview |
|---|---|
| backend/api/dependencies.py | The mode- and configuration-aware 403 detail uses the same normalized API-key source as credential validation, completing the prior PIN-only fix. |
| frontend/src/api/client.ts | Admin-gate 403 routing and send-time token comparison preserve newly exchanged sessions while retaining normal handling for unrelated 403 responses. |
| frontend/src/api/client.test.ts | Tests cover admin-gate routing, unrelated 403 handling, and the stale-response session race. |
| tests/test_loopback_server_mode.py | Tests pin desktop, keyed-server, and PIN-only server detail behavior across both admin dependencies. |
| tests/test_auth_gate_detail_lockstep.py | Cross-layer tests enforce that only key-satisfiable backend details match the frontend authentication discriminator. |
| docs/api-auth.md | Authentication documentation reflects the mode-distinct admin-gate response contract. |
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'upstream/m..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e-403 race Greptile P1 #1 (unsatisfiable PIN-only gate): in server mode with a share PIN but no OMNIVOICE_API_KEY, the PIN closes read-only bootstrap while no key exists to present — the key-naming detail sent the browser to a login form whose exchange can never succeed, hiding the whole app behind an unrecoverable gate. _admin_gate_403() now names the key only when server mode AND an API key is configured; PIN-only and bare servers answer the plain loopback detail, which is truthful (only loopback can use admin when no key exists) and keeps the SPA out of the gate. Greptile P1 debpalash#2 (stale 403 clears new session): a late 403/401 landing after a key exchange wiped the fresh session via the unconditional clearAdminSession(), reloading a successful login straight back into the gate. A failed response may now only invalidate the credentials it actually carried (send-time session capture + token compare before clearing). Also adds the docstrings CodeRabbit's coverage check asked for on the new test functions.
Contributor
Author
|
The red Tests (backend + frontend) check is pre-existing on |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1568
What
client.ts): a 403 whose detail names the admin API key now routes into the existingov:auth-requiredflow (mode: 'apikey'), exactly like the 401 paths —RemoteAuthGateshows the API-key form, the app tree unmounts, and the infinite-retry polling stops instead of spamming the console. Other 403s (CSRF "browser origin rejected", desktop-only routes) deliberately stay plain errors, because presenting a key cannot satisfy them.dependencies.py): new_admin_gate_403()emits a mode-distinct detail. Server mode keeps"loopback origin or admin API key required"(a key genuinely satisfies the gate there); desktop mode now answers"loopback origin required"— truthful, since the admin-credential checks only run under_server_mode(), so no presented key can ever satisfy the desktop gate. Without this split, routing on the message alone would trap a desktop PIN-share guest in a login form that can never succeed.tests/test_auth_gate_detail_lockstep.py): pins the literal contract across layers — the frontend matcher ("admin api key") must keep matching the backend's server-mode detail and must NOT match the details a key cannot fix. A backend reword without updatingclient.tsfails CI instead of silently regressing to 403 spam.Tests
client.test.ts: admin-gate 403 dispatches{mode:"apikey"}; CSRF 403 does not dispatch (fail-before/pass-after).test_loopback_server_mode.py: per-mode detail forrequire_admin+require_admin_action(desktop → plain loopback, server+key → key-naming).test_engines_route_shape.pyto the new truthful detail.main: backend 208 passed, frontend api 95 passed;oxfmt --checkclean.Verification
On a live LAN deployment (server mode +
OMNIVOICE_API_KEY): before, a remote browser logged/sysinfo,/model/status,/system/notifications403s every few seconds indefinitely. After: 3 initial 403s, the API-key gate renders, the exchange produces a short-lived session, and all polls return 200.An adversarial review pass caught the first version of the frontend-only fix breaking desktop LAN-share guests (unsatisfiable key form); the mode-distinct backend detail is the root-cause fix for that class.
Server-mode admin 403 responses now trigger API-key authentication, while unrelated 403 responses remain errors and stale responses cannot clear newer sessions. Backend details now reflect mode and API-key availability, with cross-layer tests covering routing and recovery. Review the pre-existing watermark route coverage failure separately; mismatched error details could still break authentication routing.
Review follow-ups (commit e2bfeaa)
Greptile's review on 014e18c flagged two P1s, both fixed with fail-before/pass-after tests:
dependencies.py): server mode + share PIN + noOMNIVOICE_API_KEYclosed read-only bootstrap while no key existed to present, so the key-naming detail gated the whole UI behind a form whose exchange can never succeed._admin_gate_403()now names the key only when server mode and an API key is configured; PIN-only and bare servers answer the plain loopback detail (truthful: only loopback can use admin when no key exists), which the SPA leaves a plain error. New test:test_require_admin_pin_only_server_mode_detail_is_plain_loopback.client.ts): a late 403/401 landing after a key exchange wiped the fresh session (clearAdminSession()was unconditional), reloading a successful login straight back into the gate. A failed response may now only invalidate the credentials it actually carried (send-time session capture + token compare). New test:a stale 403 does not clear a session stored during its flight.CodeRabbit's docstring-coverage warning on the new test functions is also addressed.